apply -UNDEBUG to coupt flags instead of global settings#964
apply -UNDEBUG to coupt flags instead of global settings#964tmckayus wants to merge 2 commits intoNVIDIA:release/26.04from
Conversation
tmckayus
added
bug
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMoves Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
|
/ok to test b2eddb2 |
|
The previous way of doing this added -UNDEBUG to everything built under the cpp directory, including fetched dependencies, if assert mode was enabled. This change applies it only to the cuopt specific compilation flags. |
| if(DEFINE_ASSERT) | ||
| add_definitions(-DASSERT_MODE) | ||
| list(APPEND CUOPT_CXX_FLAGS -UNDEBUG) | ||
| list(APPEND CUOPT_CUDA_FLAGS -UNDEBUG) |
There was a problem hiding this comment.
does this turn off all assertions?
There was a problem hiding this comment.
This is "undefine no debug", which means "leave NDEBUG (no debug) unset". So if DEFINE_ASSERT is set, we are allowing assertions guarded by NDEBUG to still work. The standard C++ assert macro checks NDEBUG to turn off assertions; cuopt_assert() delegates to assert() so this logic applies.
The only difference here is that previously if DEFINE_ASSERT was on, we set -UNDEBUG on every build under cpp, including fetched dependencies built as subdirectories. In this setup, we only set -UNDEBUG on cuopt targets so that if DEFINE_ASSERT is on we get the assertions in force.
5 participants
don't pass -UNDEBUG to other libraries we may build